fix: handle empty notification recipients#7589
Conversation
|
@SahilJat is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Code Review
This pull request introduces checks to handle cases where no recipient emails are found for API usage and blocked notifications, including a new unit test for this scenario. Feedback suggests that returning early without creating a notification record will lead to redundant processing and log noise in subsequent task runs, as the organization will be re-evaluated every 12 hours.
|
@SahilJat thanks for your contribution. You'll need to run |
2da8be0 to
07d4f78
Compare
|
Hi @Zaimwa9 i did run make lint and it shows around 22k lines edited , is this fine? |
|
Sorry, I should have been more specific as the command should be run from |
001cf1e to
f44aa4e
Compare
|
hi @Zaimwa9 thanks for the hint , i did the changes required now, please have a look and let me know. |
for more information, see https://pre-commit.ci
f44aa4e to
1add2f6
Compare
21991e4 to
1fcd6f8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7589 +/- ##
==========================================
- Coverage 98.57% 98.42% -0.15%
==========================================
Files 1464 1464
Lines 56625 56651 +26
==========================================
- Hits 55818 55760 -58
- Misses 807 891 +84 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @Zaimwa9 all the test cases are passing and test coverage is covered too. Thanks |
Zaimwa9
left a comment
There was a problem hiding this comment.
Sorry for the back and forth. Gemini comment is worth addressing
5a1373e to
7c75218
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
f00f7b1 to
f33ef16
Compare
…00-error # Conflicts: # api/custom_auth/serializers.py
Zaimwa9
left a comment
There was a problem hiding this comment.
Thanks for the change and sorry for the back and forth around marking the notification as sent.
A couple of comments, the changes in the custom auth serializer don't look like they belong to this PR though.
| ) | ||
|
|
||
| # Then | ||
| print(response.content) |
There was a problem hiding this comment.
| print(response.content) |
| "sign_up_type", | ||
| ) | ||
| read_only_fields = ("is_active", "uuid", "marketing_consent_given") | ||
| write_only_fields = ("sign_up_type",) | ||
| extra_kwargs = { | ||
| "sign_up_type": {"required": False}, | ||
| "email": { | ||
| "validators": list[object](), | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
I don't see how this is related to the issue ?
|
|
||
| recipient_emails = list(recipient_list.values_list("email", flat=True)) | ||
| if not recipient_emails: | ||
| logger.warning( |
There was a problem hiding this comment.
We might want to make it less verbose like
logger.warning(
"notification.skipped",
organisation__id=organisation.id,
reason="no_recipients",
notification_type="flags_blocked",
)
| logger.warning( | ||
| "notification.no_recipients", | ||
| organisation__id=organisation.id, | ||
| matched_threshold=matched_threshold, | ||
| ) |
There was a problem hiding this comment.
and here we could log this way
logger.warning(
"notification.skipped",
organisation__id=organisation.id,
reason="no_recipients",
)
| OrganisationAPIUsageNotification.objects.create( | ||
| organisation=organisation, | ||
| percent_usage=matched_threshold, | ||
| notified_at=timezone.now(), | ||
| ) |
There was a problem hiding this comment.
We shouldn't create a record here. It would wrongly marked it as notified. Meaning it won't be retried especially if a user is added to the mailing list.
If we just log and return, next time the task runs it will try again. Costs a bit of repeated work but as these notifications are critical. It is better to leave it self-heal
There was a problem hiding this comment.
I apologize as I trusted Gemini comment to early in the last review. That's on me
Thanks for submitting a PR! Please check the boxex below:
I have read the Contributing
Guide.
I have added information to
docs/if requiredso people know about the feature.
I have filled in the "Changes" section below.
I have filled in the "How did you test this
code" section below.
Changes
Closes Sendgrid API calls raising HTTP 400 Bad Requests #7353
This PR resolves an issue where the background tasks for sending API usage and flag blocked notifications crash with a
BadRequestsError: HTTP Error 400: Bad Request.This occurs when using external API-based email
backends (like SendGrid or AWS SES) and the
organisation has no valid recipients (e.g., zero
total users, or no admin users when the usage
threshold is <100%). Unlike traditional SMTP
backends, API backends strictly validate the payload and return a 400 error if the
recipient_listis empty.Changes made:
Added an early return guard checking
if not recipient_emails:in_send_api_usage_notification.Added the same early return guard in
send_api_flags_blocked_notificationto prevent the same crash there.Logs a warning (
notification.no_recipients)instead of attempting to send the email.
How did you test this code?
test_handle_api_usage_notifications__no_admin_users_ _skips_notificationto explicitly verify that thenotification is safely skipped and no 400 error is thrown when an organisation has no users.
organisations_tasksto ensure no regressions.